Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M2-6044: Submit Take Now Params #441

Merged
merged 2 commits into from
Apr 30, 2024

Conversation

sultanofcardio
Copy link
Contributor

@sultanofcardio sultanofcardio commented Apr 22, 2024

📝 Description

🔗 Jira Ticket M2-6044

This PR updates the code that submits the activity responses to /answers to include the values for target_subject_id and source_subject_id. The values are only included if the enableMultiInformant feature flag is enabled.

📸 Screenshots

N/A

🪤 Peer Testing

Note

This feature depends on the enableMultiInformant feature flag. If it it not already enabled, you can manually enable it here like so:

features.enableMultiInformant = true;

Note

There's an in-progress API patch that this PR depends on. If ChildMindInstitute/mindlogger-backend-refactor#1264 is still open when you are reviewing this PR, please use that branch as the basis for your API. Otherwise, you should use the feature/multiinformant-metapod branch

Follow the peer testing steps in #438 and complete the activity that is opened with the network tab of dev tools open.
Look for the answer submission request and confirm that it includes the two fields sourceSubjectId and targetSubjectId:

Screenshot 2024-04-22 at 3 44 33 PM

✏️ Notes

N/A

@sultanofcardio sultanofcardio self-assigned this Apr 22, 2024
@sultanofcardio sultanofcardio force-pushed the feature/M2-6042-validate-take-now-params branch from 455e1a8 to 20d4334 Compare April 23, 2024 21:35
@sultanofcardio sultanofcardio force-pushed the feature/M2-6044-submit-take-now-params branch 3 times, most recently from ace3725 to 10598bf Compare April 25, 2024 05:14
Base automatically changed from feature/M2-6042-validate-take-now-params to dev April 25, 2024 18:04
@sultanofcardio sultanofcardio force-pushed the feature/M2-6044-submit-take-now-params branch from 10598bf to d1271c6 Compare April 25, 2024 18:05
@sultanofcardio sultanofcardio marked this pull request as ready for review April 25, 2024 18:06
@ChaconC
Copy link
Contributor

ChaconC commented Apr 26, 2024

Which branch of the backend do I need to test this with? I'm using feature/multiinformant-metapod and it's rejecting the request:
CleanShot 2024-04-26 at 12 02 27
CleanShot 2024-04-26 at 12 03 02

The params are added as expected
CleanShot 2024-04-26 at 12 03 38

@sultanofcardio
Copy link
Contributor Author

Which branch of the backend do I need to test this with? I'm using feature/multiinformant-metapod and it's rejecting the request:

@ChaconC what's in the body of the error response?

@ChaconC
Copy link
Contributor

ChaconC commented Apr 26, 2024

Which branch of the backend do I need to test this with? I'm using feature/multiinformant-metapod and it's rejecting the request:

@ChaconC what's in the body of the error response?

"Subject 7d00095c-cc5b-442b-bcbe-a656d57f50db not found"

Looks like a problem when using take now on a Limited account. Full accounts work ok

@sultanofcardio
Copy link
Contributor Author

Looks like a problem when using take now on a Limited account.
Full accounts work ok

I'm not sure what the behaviour of the API should be in that case, but this flow doesn't support using a limited account as the sourceSubjectId. So that's a scenario we can ignore for now

ChaconC
ChaconC previously approved these changes Apr 29, 2024
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @sultanofcardio, I'm currently struggling to get this to work. There may be some user error on my part, so I'm wading through that currently. But I've also discovered that it seems sourceSubjectId may not always be getting correctly initialized on the multiInformantState when the quiz starts (so not related to the code in this PR). It's time-consuming to figure out what's going on, so please bear with me. I just wanted to let you know before you merge so that in case I've discovered a bug, you can include the fix in this PR.

I'll keep you posted!

@ChaconC ChaconC dismissed their stale review April 29, 2024 16:04

Reviewing errors we're seeing

… flow

We can get stuck in a weird state where the values in the state won't get updated, even if the new values from the params are different. By also comparing the values themselves, we can break out of this state
@sultanofcardio sultanofcardio requested a review from ChaconC April 29, 2024 20:38
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After troubleshooting and figuring creating a workaround for a BE bug as mentioned in Slack, and testing again with your changes, all tests are now working. Thanks @sultanofcardio! 💪🏻

@sultanofcardio sultanofcardio merged commit 3b70529 into dev Apr 30, 2024
5 checks passed
@sultanofcardio sultanofcardio deleted the feature/M2-6044-submit-take-now-params branch April 30, 2024 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants